Skip to content

Comments

cmake: fix a few constraints in dependents#3259

Merged
alalazo merged 6 commits intodevelopfrom
hs/fix/cmake-build-dep
Feb 16, 2026
Merged

cmake: fix a few constraints in dependents#3259
alalazo merged 6 commits intodevelopfrom
hs/fix/cmake-build-dep

Conversation

@haampie
Copy link
Member

@haampie haampie commented Feb 5, 2026

Fixes a couple oversights that may or may not prevent unified concretization of
build dependencies in the Windows stack.

@haampie haampie requested a review from a team as a code owner February 5, 2026 12:41
@haampie haampie added the pipelines:urgent Skip "deferred pipelines" check. Only use if rebasing on a tested develop commit is unfeasible label Feb 5, 2026
kwryankrattiger
kwryankrattiger previously approved these changes Feb 5, 2026
Copy link
Contributor

@kwryankrattiger kwryankrattiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@spackbot-triage spackbot-triage bot added the ci Issues related to Continuous Integration label Feb 5, 2026
@haampie haampie force-pushed the hs/fix/cmake-build-dep branch from cc80851 to 671997b Compare February 6, 2026 09:25
@spackbot-triage spackbot-triage bot removed the ci Issues related to Continuous Integration label Feb 6, 2026
@haampie haampie changed the title curl/libuv/scitokens_cpp: cmake is build dep cmake: fix a few constraints in dependents Feb 6, 2026
@haampie
Copy link
Member Author

haampie commented Feb 6, 2026

when="%cxx=msvc %fortran=msvc" seems to be triggering a bug in the concretizer.

Now testing with the fix included from spack/spack#51923.

Edit: seems to work, reverting that...

@haampie haampie force-pushed the hs/fix/cmake-build-dep branch from 76fa0b6 to 7691c31 Compare February 12, 2026 14:34
@haampie haampie removed the pipelines:urgent Skip "deferred pipelines" check. Only use if rebasing on a tested develop commit is unfeasible label Feb 12, 2026
@spackbot-triage spackbot-triage bot requested a review from alecbcs February 12, 2026 16:30
@spackbot-triage spackbot-triage bot requested a review from alecbcs February 12, 2026 16:40
alecbcs
alecbcs previously approved these changes Feb 13, 2026
Copy link
Member

@alecbcs alecbcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the quick turnaround @haampie :)

@alalazo I'll wait on your final review before we merge since you previously requested changes.
(And thus I'd have to force merge in the GitHub interface.)

* use `type=build` where appropriate
* avoid a "global" `conflicts("cmake`, use `depends_on("cmake@...`
  instead

Signed-off-by: Harmen Stoppels <me@harmenstoppels.nl>
Signed-off-by: Harmen Stoppels <me@harmenstoppels.nl>
Signed-off-by: Harmen Stoppels <me@harmenstoppels.nl>
@haampie haampie force-pushed the hs/fix/cmake-build-dep branch from 2c838b8 to 545665b Compare February 13, 2026 08:55
@spackbot-triage spackbot-triage bot requested a review from alecbcs February 13, 2026 08:56
@haampie
Copy link
Member Author

haampie commented Feb 13, 2026

Rebased this to include the latest Spack version to deal with the concretization issue...

alalazo
alalazo previously approved these changes Feb 13, 2026
@alalazo alalazo enabled auto-merge (squash) February 13, 2026 09:12
Signed-off-by: Harmen Stoppels <me@harmenstoppels.nl>
@haampie haampie dismissed stale reviews from alalazo and alecbcs via c89894a February 13, 2026 11:15
@haampie
Copy link
Member Author

haampie commented Feb 13, 2026

@johnwparent for your information: the last commit removes your constraint that requires cmake@4.1: on windows when cxx,fortran=msvc. The reason for this:

  • Previously this constraint was ignored by the concretizer due to a bug, so after removal the status quo is maintained.
  • If the constraint is respected, it makes concretization of paraview impossible because it requires both external cmake@3 and cmake@4.1: with paraview %cxx,fortran=msvc

Maybe you could bump external CMake to a newer version in the relevant image and add back the constraint?

IMO the change shouldn't be controversial given that the conflict was ignored from the start and paraview built fine.

@johnwparent
Copy link
Contributor

johnwparent commented Feb 13, 2026

There's also always the potential that updating the image might cause other CI failures, so if this is an immediate need, it makes sense to just merge without trying to update the CI image.


I don't love actively introducing a broken relationship between CMake and %fortran=msvc but as Harmen points out, it's a no-op functionally so 🤷‍♂️ .

We also must not actually be using the fortran compiler with CMake in CI, since otherwise I have no idea how that's working... maybe there's a lower bound where oneAPI + MSVC + CMake work again...

The correct solution, as Harmen points out, is to bump the CMake version in the CI image to post 4.1 (and for me to investigate whether there is a lower bound for this conflict) but that may take some time if the image updates introduces other failures.

Thanks for the heads up @haampie

Signed-off-by: Harmen Stoppels <me@harmenstoppels.nl>
@haampie
Copy link
Member Author

haampie commented Feb 13, 2026

Just commented out the constraint so that we don't forget about it

@alalazo alalazo merged commit 97e2b85 into develop Feb 16, 2026
27 checks passed
@alalazo alalazo deleted the hs/fix/cmake-build-dep branch February 16, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants